Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic file loaders #214

Closed
wants to merge 3 commits into from
Closed

Conversation

indygreg
Copy link

@indygreg indygreg commented Jun 4, 2023

Thanks for this awesome crate!

Like many other users have reported via GitHub issues, I've run into various limitations where the built-in TOML or trycmd file functionality in this crate just isn't powerful enough for my use cases and I'm yearning for missing features.

Adding new features to the test file formats obviously needs to be done with care. It's easy to make bad decisions and be saddled with those mistakes for years. So I understand why many features aren't yet implemented.

This got me thinking: how can I start to experiment with more advanced test file format features without forking this crate or having to reinvent the functionality myself. This PR is my proposed solution.

In a nutshell this PR:

  1. Makes TryCmd, Step, Filesystem, and Env and all their fields public so TryCmd can be instantiated outside of this crate.
  2. Adds a public API to TestCases that allows providing your own function for loading a file into a TryCmd.

Using the new dynamic file loading API, you can define your own extensions / formats or override the built-in ones.

In my opinion this change drastically increases the value of this crate because it enables experimentation with alternative test file formats without having to throw away the test runner. For example, I plan to experiment with modifying the trycmd parser to introduce syntax extensions, change semantics around the filesystem sandbox, etc. This type of experimentation is not possible today without a fork since there's no way to customize file loading.

indygreg added 2 commits June 4, 2023 16:05
I feel like there is sufficient complexity here to warrant isolating
each extension's handling to its own function. So do that.

As part of this we also make the new functions public so they can
be called by external crates. Combined with a future commit to
make file loaders configurable, this will enable external crates
to wrap file loading of built-in formats and modify constructed
`TryCmd` instances as they deem appropriate.
The `TryCmd` struct, its fields, the types it references and all
their fields are now `pub` instead of `pub(crate)`. This allows
end-users to construct instances using whatever mechanism they see
fit. This opens the door to allowing end-users to define their own
file formats without requiring the cooperation of the `trycmd`
crate.
@indygreg indygreg force-pushed the dynamic-file-loaders branch from b9efa38 to 89a969b Compare June 4, 2023 23:08
indygreg added a commit to indygreg/apple-platform-rs that referenced this pull request Jun 19, 2023
I love this tool but it isn't powerful enough to facilitate the kinds
of tests I want to write.

I have a PR at assert-rs/snapbox#214 to
add functionality that allows you to extend trycmd's file formats.
This commit references my GitHub fork with this new feature.
Before, there was a static mapping of supported file extensions
to the functions that loaded them. It wasn't possible to define
your own file formats or to customize the loading of the built-in
extensions by providing your own code. This commit changes that.

File loading now takes a mapping of file extensions to functions to
load them. This mapping is consulted to determine how to load each
encountered file.

The public `TestCases` struct gains a new public API to register
a custom loader for a file extension.

This feature enables end-users to leverage the power of the `trycmd`
runner without being constrained to the built-in semantics for TOML
and `trycmd` file handling.
@indygreg indygreg force-pushed the dynamic-file-loaders branch from 89a969b to 32e9de6 Compare June 19, 2023 16:35
indygreg added a commit to indygreg/apple-platform-rs that referenced this pull request Jun 19, 2023
I love this tool but it isn't powerful enough to facilitate the kinds
of tests I want to write.

I have a PR at assert-rs/snapbox#214 to
add functionality that allows you to extend trycmd's file formats.
This commit references my GitHub fork with this new feature.
indygreg added a commit to indygreg/apple-platform-rs that referenced this pull request Jun 20, 2023
I love this tool but it isn't powerful enough to facilitate the kinds
of tests I want to write.

I have a PR at assert-rs/snapbox#214 to
add functionality that allows you to extend trycmd's file formats.
This commit references my GitHub fork with this new feature.
@epage
Copy link
Contributor

epage commented Jun 23, 2023

Sorry the delay on getting to this.

I have to say this is a big change and I prefer these things to be discussed in issues first before we move onto PRs

  • Less wasted time and frustration on your side if we need to go a different direction
  • Keeps the conversation in place on problem/requirements/etc if multiple PRs are used
  • Frankly, PRs put extra social pressure on maintainers to "move things forward" regardless of whether they shouldn't or not and how much extra work I have to put into it and its healthier for myself as a maintainer to instead consistently push back on PRs, encouraging Issues instead.

@indygreg
Copy link
Author

Thanks for replying.

As a fellow maintainer I understand your position and want to work with you to get this functionality merged.

Let me explain my perspective so you can understand where I'm coming from.

I'm in need of a trycmd like solution for expressive CLI snapshot testing and trycmd in its current form doesn't quite cut it. I was thinking that the ideal outcome would be extending trycmd to support the features I need. The less ideal solution would be forking trycmd (hopefully a friendly, temporary fork!). Either way I needed to write code to demonstrate my ideas were viable. I wrote that code. Hence this PR.

You expressed your preference that big changes start first in issues. The horse has already left the stable. Could you please clarify next steps? Do you want me to open an issue for discussion? Close this PR? Please tell me what you want and I'll do it.

Also, I want to be transparent that I'm already relying on functionality in this PR in a project of mine. I'm currently pulling trycmd from my personal GitHub repo. I'm optimistic this PR's functionality can get merged into official trycmd. But if I need to release and publish to crates.io before then, I think that means I'll be forced to publish a fork of trycmd to crates.io. I don't want to do that. But that's what crates.io forces you to do. From my perspective it seems the least bad option from the alternatives of a) not releasing b) not using expressing CLI snapshot testing c) rolling my own CLI snapshot testing crate.

@epage
Copy link
Contributor

epage commented Jun 26, 2023

You expressed your preference that big changes start first in issues. The horse has already left the stable. Could you please clarify next steps? Do you want me to open an issue for discussion? Close this PR? Please tell me what you want and I'll do it.

No, it hasn't as I've not made any comments on this except for procedural ones. Please, let's start this in an issue as I requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants